Skip to content

Conversation

@wgy035
Copy link
Contributor

@wgy035 wgy035 commented Aug 7, 2024

Hopefully resolves #11887

Remove reduntant attributes combination in OperationListener.onEnd, move all attributes extract to AttributesExtractor.onEnd

@wgy035 wgy035 requested a review from a team August 7, 2024 08:21
@wgy035 wgy035 changed the title Remove Attributes combanation from onEnd Remove Attributes combination from onEnd Aug 7, 2024
if (operationListeners.length != 0) {
long endNanos = getNanos(endTime);
for (int i = operationListeners.length - 1; i >= 0; i--) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The modification here is useless.

Copy link
Contributor

@steverao steverao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some CI failures, you can solve them by referring to https://gradle.com/s/7qpfxdz7iymuy


@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
super.onStart(attributes, parentContext, request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the attributes that are added at the start will be available for sampling. Moving all the attributes to the onEnd will make sampling impossible.

Copy link
Contributor Author

@wgy035 wgy035 Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I need to figure out which attributes impact sampling, and those attributes are the minimum set need to retain, right?

According to semantic-conventions(e.g. http-spans), I'd like to extract the unique attributes of span at the start without needing to pass them on. The common attributes attributes should be extracted at the end.

@laurit
Copy link
Contributor

laurit commented Aug 7, 2024

Also see #11092

@laurit
Copy link
Contributor

laurit commented Aug 8, 2024

Did you consider using something like

public final class CompositeAttributes implements Attributes {
  private final Attributes first;
  private final Attributes second;

  private CompositeAttributes(Attributes first, Attributes second) {
    this.first = first;
    this.second = second;
  }

  public static Attributes of(Attributes first, Attributes second) {
    return new CompositeAttributes(first, second);
  }

  @Nullable
  @Override
  public <T> T get(AttributeKey<T> attributeKey) {
    T value = first.get(attributeKey);
    if (value != null) {
      return value;
    }
    return second.get(attributeKey);
  }

  @Override
  public void forEach(BiConsumer<? super AttributeKey<?>, ? super Object> biConsumer) {
    first.forEach(biConsumer);
    second.forEach(biConsumer);
  }

  @Override
  public int size() {
    return first.size() + second.size();
  }

  @Override
  public boolean isEmpty() {
    return first.isEmpty() && second.isEmpty();
  }

  @Override
  public Map<AttributeKey<?>, Object> asMap() {
    Map<AttributeKey<?>, Object> map = new HashMap<>();
    map.putAll(first.asMap());
    map.putAll(second.asMap());
    return map;
  }

  @Override
  public AttributesBuilder toBuilder() {
    return first.toBuilder().putAll(second);
  }
}

@laurit
Copy link
Contributor

laurit commented Aug 16, 2024

@wgy035 have you had a chance to check whether composing the start and end attributes as described above would help?

@wgy035
Copy link
Contributor Author

wgy035 commented Aug 17, 2024

@wgy035 have you had a chance to check whether composing the start and end attributes as described above would help?

@laurit yep, using such a composite structure will reduce traverse overhead and approximately a CompositeAttributesBuilder is also needed, now I'm testing the performance improvement and the code will be updated with a new commit.

@wgy035 wgy035 force-pushed the instrumenter-metrics-optimize branch from e023041 to 41b9c6e Compare September 13, 2024 09:19
@github-actions
Copy link
Contributor

This PR has been labeled as stale due to lack of activity. It will be automatically closed if there is no further activity over the next 14 days.

@github-actions github-actions bot added the stale label Sep 29, 2025
@github-actions github-actions bot closed this Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Could we avoid attributes merge in HttpServerMetrics.onEnd?

3 participants